Skip to content

Conversation

typotter
Copy link
Collaborator

@typotter typotter commented Feb 14, 2025


labels: mergeable

Towards FF-3886

Motivation and Context

The configuration served now contains the format field which indicates whether it is an obfuscated configuration payload. This change is made as part of ongoing efforts to clean up the construction and initialization of the EppoClient (and sub-classes) to allow for multiple instance of the EppoClient. . isObfuscated is a low-hanging fruit.

Description

  • Deprecate the isObfuscated param in EppoClientParameters
  • interpolate whether the config is obfuscated by checking the format field (set onto the flag configuration store when the response is received).
  • maintain the existing public setObfuscated api, but track the variable as expectObfuscated and
  • log a warning if the expected does not match the actual when assignments are evaluated.
  • cache whether the config is obfuscated; clearing that cache on a best effort basis (where the config is actually updated is not observable from the EppoClient, so we invalidate when a new flag config store is set, and when the config is fetched)
  • cache whether the client issued a warning about obfuscation mismatch. This cache is cleared when the configuration enters a good state (expected and actual obfuscation match), so that if a mismatched state is (re-)entered, a warning will still be issued.

How has this been tested?

  • The existing tests required modification to ensure the configuration format was set when accessing the configuration store directly for tests.

@typotter typotter changed the title chore: inspect format field to determine whether config is obfuscated feat: inspect format field to determine whether config is obfuscated Feb 14, 2025
@typotter typotter force-pushed the tp/deprecate-obfuscated branch from 8158e45 to 8aee608 Compare February 14, 2025 03:46
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!


// original configuration version
storage.setEntries({ [flagKey]: mockFlag });
setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not awaiting some of these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good catch. These tests have still worked because the underlying Flag Config Storage is a memory-only store, so the actual "writing" of entries is happening sync when the method is called.
Worth it to fix it now before it causes weird problems in the future.


private isObfuscated() {
const configFormat = getFormatFromString(this.flagConfigurationStore.getFormat());
const configObfuscated = OBFUSCATED_FORMATS.includes(configFormat);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@typotter
Copy link
Collaborator Author

Thanks for the review, @felipecsl and @aarsilv . I'll leave this up for the day to give you a chance to comment on the caching if you'd like.
Cheers

/**
* UFC Configuration formats which are obfuscated.
*/
export const OBFUSCATED_FORMATS: string[] = [FormatEnum.CLIENT, FormatEnum.PRECOMPUTED];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this string[] instead of the interpolated FormatEnum[] allows us to check if the config wire value (a string) is one of these formats without having to convert from string to enum (which actually requires a lookup or array iteration).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this is actually true as FormatEnum is a string enum, so there's no lookup involved when casting string to enum. Check JS tab herestringToEnum is a noop

(Even though there's no performance penalty for casting string to enum, doing so is type-unsafe as the string could be anything, including values not listed in enum.)

@typotter typotter enabled auto-merge (squash) February 19, 2025 19:00
@typotter typotter merged commit ffa30b4 into main Feb 19, 2025
8 checks passed
@typotter typotter deleted the tp/deprecate-obfuscated branch February 19, 2025 19:00
this.overrideStore = overrideStore;
this.configurationRequestParameters = configurationRequestParameters;
this.isObfuscated = isObfuscated;
this.expectObfuscated = isObfuscated;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 This looks like it sets expectObfuscation (to either true or false) even if user didn't pass any value, which may cause an unexpected warning

this.expectObfuscated = isObfuscated;
}

private maybeWarnAboutObfuscationMismatch(configObfuscated: boolean) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we develop elaborate warning for a deprecated flag?

  1. The warning mechanism is sufficiently complicated that there's a bug in it (see above comment).
  2. If we think this kind of warning is useful enough to justify mechanism, it makes sense to keep it and not deprecate the flag (or add expectObfuscation flag). (Although I don't think it's worth it.)
  3. If it's not that useful, I would rather remove it completely. Maybe issue a quick deprecation warning in constructor when isObfuscated is supplied

Comment on lines +181 to +187
if (this.configObfuscatedCache === undefined) {
this.configObfuscatedCache = OBFUSCATED_FORMATS.includes(
this.flagConfigurationStore.getFormat() ?? FormatEnum.SERVER,
);
}
this.maybeWarnAboutObfuscationMismatch(this.configObfuscatedCache);
return this.configObfuscatedCache;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: maintaining this cache is probably as costly as doing two comparisons (we actually only need one as precomputed format is not valid here).

major: cache invalidation is also dubious and there seems to be a bug lurking in it. Cache is reset before issues a configuration fetch. However, as fetch is async, the cache will get re-populated if evaluation is requested, and subsequent configuration fetch success does not invalidate the cache. This may cause an evaluation bug if initial configuration obfuscation does not match remote configuration obfuscation (e.g., client initialized with non-obfuscated server config first and fetches an obfuscated configuration later)

If we want to cache obfuscation status, configuration itself is the right place to do that

/**
* UFC Configuration formats which are obfuscated.
*/
export const OBFUSCATED_FORMATS: string[] = [FormatEnum.CLIENT, FormatEnum.PRECOMPUTED];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this is actually true as FormatEnum is a string enum, so there's no lookup involved when casting string to enum. Check JS tab herestringToEnum is a noop

(Even though there's no performance penalty for casting string to enum, doing so is type-unsafe as the string could be anything, including values not listed in enum.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants